Fix inlining of references to external files - #27
Merged
Merged
Conversation
…file
`isInlinable` judged a ref by the last link of its alias chain instead of
by the ref's own target. When that chain ended in an external file (or a
missing target), a ref into a removed or shifted part was kept as written:
- 3.1 -> 3.0: `/pets: {$ref: '#/components/pathItems/Pets'}` with
`Pets: {$ref: './paths/pets.yaml'}` kept pointing at the removed
`components.pathItems`.
- `$defs` entries that alias external files were left dangling the same way.
- In a list that lost an entry, the kept ref silently resolved to the
entry that shifted into its slot (e.g. a `secret` header instead of
`Limit`).
Base the check on the ref's own target. Keep refs whose alias chain loops
as written, as before: they have no target to inline, and following them
would never end in `mergeRef`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011uRMv7RDjXFSBo71sjcefJ
Keep one case per converter path instead of several that exercise the same engine branch: drop the duplicate `$defs` assertion and the second path item entry, fold the shifted external parameter into the existing shifted-list test, and merge the removed-loop case into the existing alias-loop test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uRMv7RDjXFSBo71sjcefJ
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
isInlinableindowngrader/src/shared.ts: now checks the ref's immediate target (resolveRef(ref)is a record or boolean) plus thataliasEnd(ref)terminates, instead of resolving the chain's terminal target. Refs whose target is itself a$refto an external file are now inlinable, so they no longer survive as dangling references inside removed parts.- Regression tests:
shared.test.tsgains a removed-alias-to-external case;v3.1-to-v3.0.test.tsgainscomponents.pathItemsand$defsentries pointing at external files; thev3.2-to-v3.1.test.tsparameter-shift test now inserts an external entry soShiftedrebases.
I confirmed the four added/modified tests fail against the previous isInlinable condition and that the full suite (487 tests) passes at 21dc45e. Cycles remain non-inlinable because aliasEnd returns undefined for them, and direct external refs still stay as written. No README change needed — line 139 already documents "following the reference chain until it leaves the removed part".
DeepSeek Flash (default — pick a model for stronger reviews) ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a bug in the reference inlining logic where references to external files were not being properly inlined when they were themselves targets of internal references.
Key Changes
Fixed
isInlinablelogic inshared.ts: Changed the condition to check if a reference resolves to an inlinable target (record or boolean) AND has a valid alias end, rather than checking the alias end first. This ensures that references to external files (which have no alias chain) are correctly identified as inlinable when they are direct targets.Added test coverage for external file references:
v3.1-to-v3.0.test.tsto verify path items referencing external files are inlined correctlyv3.1-to-v3.0.test.tsto verify schema$defsentries referencing external files are inlined correctlyshared.test.tsto verify references whose targets are themselves external references are properly inlinedv3.2-to-v3.1.test.tsto include external file reference scenariosImplementation Details
The core issue was in the
isInlinablefunction which was usingaliasEnd(ref)to determine if a reference could be inlined. However,aliasEndreturnsundefinedfor references that don't have an alias chain (like direct external file references), causing them to be incorrectly marked as non-inlinable. The fix reorders the logic to first resolve the reference and check if it's a valid target, then verify it has a proper alias chain, allowing external file references to be inlined when appropriate.https://claude.ai/code/session_011uRMv7RDjXFSBo71sjcefJ